Conversation
- SMProAssayCreateTest.testAddResultFields - SMSampleCreateTest.testSamplesWithMultiChoice - SMSourceTypeMultiChoiceTest - ListTest.testMultiChoiceValues
…ag() fix test testMultiChoiceEditInBulk()
-add new shuffleSelect -replace my method with shuffleSelect
labkey-danield
left a comment
There was a problem hiding this comment.
Added a few comments.
| if(textChoiceValidator.getMultipleSelections()) | ||
| { | ||
| fieldRow.setAllowMultipleSelections(textChoiceValidator.getMultipleSelections()); | ||
| } |
There was a problem hiding this comment.
This will only set it if textChoiceValidator.getMultipleSelections is true, correct? This is ok if you are creating a field, but wouldn't work as expected if the test is editing an existing field and wants to change to field from a multi-choice to a single choice.
Perhaps a better check would be:
if(null != textChoiceValidator.getMultipleSelections())
| lookupSelect.clearSelection(); | ||
|
|
There was a problem hiding this comment.
Will this work if a test is trying to add to an existing selections?
| if(textChoiceValidator.getMultipleSelections()) | ||
| { | ||
| fieldRow.setAllowMultipleSelections(textChoiceValidator.getMultipleSelections()); | ||
| } |
There was a problem hiding this comment.
This would make it impossible to disable multi-select through this method.
This should either call setAllowMultipleSelections unconditionally or the condition should check whether textChoiceValidator.getMultipleSelections() != null.
| /** | ||
| * Select a single facet value by clicking its label. Should replace all existing selections. | ||
| * @param value desired value | ||
| */ | ||
| public void selectFilter(String value) | ||
| { | ||
| elementCache().filterTypeSelects.select(value); | ||
| } |
There was a problem hiding this comment.
Update this comment to describe this method (looks leftover from the selectValue). The comment should also mention that this is only relevant to multi-value text choice fields.
This could also take a Filter.Operator instead of a String for some extra type-safety.
| List<String> values = (List<String>) value; | ||
| filterModal.selectFacetTab().selectValue(values.get(0)); | ||
| filterModal.selectFacetTab().checkValues(values.toArray(String[]::new)); | ||
| filterModal.selectFacetTab().selectFilter(operator.getDisplayValue()); |
There was a problem hiding this comment.
I think this will (or could) break functionality for other column types; the facet panel usually doesn't have a filter type.
Also, this wouldn't work for "Is Empty" or "Is Not Empty".
There was a problem hiding this comment.
I’m planning to add support for 'Is Empty' and 'Is Not Empty' as well.
Regarding the functionality for other types: I’ve already found one test that was failing, so I added a check to the initFilterColumn method. It now checks if the filter type selector is present on the page before trying to interact with it. After this change, the test is passing.
Does this approach sound okay to you?
| for (Map.Entry<String, String> entry : values.entrySet()) | ||
| { | ||
| WebElement fieldInput = Locator.name(EscapeUtil.getFormFieldName(entry.getKey())).findElement(getDriver()); | ||
| WebElement fieldInput = Locator.nameContaining(EscapeUtil.getFormFieldName(entry.getKey())).findElement(getDriver()); |
There was a problem hiding this comment.
Use a more precise locator and add a comment explaining why this can't match the name exactly. (See UpdateQueryRowPage)
| table.clickInsertNewRow(); | ||
| String valuesToChoose = tcValues.subList(1, 3).stream() | ||
| .sorted() | ||
| .collect(Collectors.joining(" ")); | ||
| Locator loc = Locator.nameContaining(EscapeUtil.getFormFieldName(columnName)); | ||
| selectOptionByText(loc, valuesToChoose); |
There was a problem hiding this comment.
clickInsertNewRow returns a page object that should have methods for setting fields.
| if(Boolean.parseBoolean(selectElement.getAttribute("multiple"))) { | ||
| List<WebElement> elems = selectElement.findElements(Locator.tag("option")); | ||
| elems.forEach(element->{ | ||
| if(value.contains(element.getAttribute("value")) ^ element.isSelected()) element.click(); | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This is less precise than we like for broadly available helpers; it would get confused by selection options that have overlapping text. I would suggest having a separate method for multi-select (selectOptionsByText(WebElement selectElement, List<String> values).
Co-authored-by: Trey Chadick <tchad@labkey.com>
- Test fixes for change from quoted values to bold in error messages
getConversionErrorMessage() fix to message case for invalid date/timestamp now quoting field name for consistency
Co-authored-by: Trey Chadick <tchad@labkey.com>
Co-authored-by: Trey Chadick <tchad@labkey.com>
# Conflicts: # src/org/labkey/test/pages/query/UpdateQueryRowPage.java
Rationale
Some changes to support multi choice
Related Pull Requests
Changes